-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First VBScript implementation #624
Conversation
Hi @OldLiu001 Congrats to you for making your own VBScript implementation. I made a pull request (#640) which enable the |
Thank you for your contribution. I test my impl with this script (Batch File):
It passed all the tests. |
b56ec95
to
96cda20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @OldLiu001 sorry for the very slow reply (I've been unable to work on this project for the past couple of years but I can again). If you're still interested in getting this merged upstream I would ask that you do the following:
- Rebase the code onto the current HEAD
- The recommended process has changed slightly: the eval_ast (EvaluateAST) is no longer seperate from eval (Evaluate) and macroexpand has been simplified (integrated into eval and no longer a separate function). It would be good if you could update to follow the new structure.
- Add windows runner support to the Github Actions CI process. This will mean modifying:
get-ci-matrix.py
extract the windows implementations and return ado_windows
andwindows
outputs.github/workflows/main.yaml
: create a windows job (similar to macos job) that uses 'runs-on: windows-2022' (https://github.blog/changelog/2021-11-16-github-actions-windows-server-2022-with-visual-studio-2022-is-now-generally-available-on-github-hosted-runners/). Theci.sh
command uses bash but that's available on the windows runners so there shouldn't be too many changes need to that script.
- Add you implementation to
Makefile.impls
and toIMPLS.yml
and make sure that the CI tests all pass for your implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your reply.
I'm glad you've been available to handle these pull requests lately, and I still look forward to my code being merged upstream to help more learners.
I have made the corresponding changes according to your suggestions.
- First I pulled the latest code and wrote the Github Action configuration for Windows.
- I later found that
runtest.py
didn't work on windows, so I merged the code contributed by @cy20lin in Support runtest.py to work on Windows when no_pty=True #640 and dealt with merge conflicts.
I also encountered the following problems:
- Github Action calls
ci.sh
normally,ci.sh
callsruntest.py
normally, but I found thatruntest.py
could not call the bash scriptimpls/vbs/run
properly, showingnot a runable program or batch file
. - I tried to append
bash
to the header of the command line before python calledimpls/vbs/run
(likebash ../vbs/run
), but the call still failed, and the Github Action log showed that bash command neededWSL installed
before it could be used. - It seems that in the windows environment of Github Action, bash scripts cannot call bash again after calling another scripting environment.
- I tried to search the Internet, but there was no discussion of the issue.
- So I wrote a batch script
run.cmd
that does exactly the same thing as the bash scriptrun
, and modified the logic associated withruntest.py
to default to calling the batch scriptrun.cmd
instead of the bash scriptrun
on windows. - Because the test
run_argv_test.sh
in step 6 requires calling the bash scriptrun
, I kept both versions of the script (run
&run.cmd
).
With the above actions, I successfully completed testing my VBScript implementation on the Windows environment of Github Action.
You can find details of the tests for my implementation at the bottom of the test results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OldLiu001 Apart from the runtest.py
changes, everything else looks good. The runtest.py
changes to support Windows are pretty intrusive. The program is already pretty hard to maintain and reason about.
Would you be willing to try WSL for the Windows workflow job? There is an existing action that will install WSL https://github.com/Vampire/setup-wsl. I would like to see if that would allow runtest.py to work without needing to completely rework it to run on Windows. I suspect that it should work without the run.cmd
too. If you can get this working under WSL without the major runtest.py changes, then I would consider this ready for merging.
Also, did you see my comment about the incremental process/layout changing (eval_ast and macroexpand are no longer separate functions). It's not required for getting this merged, but it would be good if you could look into that at some point.
@kanaka Thank you for your advice. I just tried the following:
Then I got:
The results show that when WSL communicates with Windows native console programs, some unexpected behavior may occur, leading to test failure. In order to be compatible with Windows, it seems that I also agree with you that patching Besides, I have taken note of incremental process / layout changing problem. But I think we should finish our discussion of the current issue and then consider some relative details. |
Can you run those tests again with |
@kanaka I did the following tests:
The results of the
Log files are as follows:
So far I have reached the following conclusions:
|
Not sure exactly how to continue, but maybe this will help. (welcome back @kanaka , glad you're okay. got me a bit worried, I must say.) |
Let me briefly summarize the above discussion:
My personal advice:
|
@kanaka Regarding the structural changes you mentioned in merging 'EvaluateAST' and 'MacroExpand' into 'Evaluate', I went to #587 , and since it's been a long time since I finished my vbs implementation, I went back to my code structure. I found it interesting that when I was writing code, I already realized that normal functions and macros were identical in evaluation (that is, they were identical except for whether the parameters were evaluated), and I used the same class to represent both structures. I implemented the evaluation procedure as two methods of the 'MalProcedure' class, 'Apply' and 'ApplyWithoutEval'. I even added a new method, 'MacroApply'. It differs from 'ApplyWithoutEval' only in that it checks whether 'Procedure' is a macro at its head, and the logic I implemented to distinguish between functions and macros was to use a different class attribute as a tag. I even unified the interfaces for 'vbs functions' and 'mal functions' and 'mal macro' calls (although the object-oriented implementation of vbs has no concept of interfaces, but it is a dynamically typed language, and two classes can be considered to implement the same interface as long as they have the same properties and methods). This has the advantage of using the same upper level code for processing. Regarding evalast merging into eval, I think this is really a good change because evalast is only called once in eval, and there is no need to implement it as a separate function. And it does integrate the logic of the code. But if you look at my implementation, you'll see that my TCO implementation is very elegant (forgive me for bragging here, lol). The core idea of my tco implementation is that if the last step a function takes is to call another function and then return to the previous layer, then we can swap the order of the two steps, that is, return to the previous layer before making the function call. I use lazy evaluation to delay EVAL in tail recursion scenarios, so separating evalast and eval does not increase the call stack or cause my code to become verbose and uncontrollable. In summary, I found that some of the ideas for merging code structures were already present in my implementation, and some didn't seem too bad for my implementation, so I thought it would be best to keep the current version. Also something I discovered while writing my implementation is that if you switch the order in which some functions are implemented in the flow, many functions can be implemented with other functions, that is, the code can contain more parts that implement mal using mal itself. Using the mal language itself to implement mal's other functions may cause the code to run less efficiently, but I think that's cool, right? |
@dubek Thanks! I'm glad to be able to spend time on this again. |
@OldLiu001 Anyways, thank for the detailed output and good thought to compare with bash. That has given me an idea for a minor change to runtest to support Windows+WSL in If it's still broken, I would be interested in the the same debug output from vbscript and bash as before. I'm pretty confident that we can get this to work without the full refactoring of |
Yeah, that's true that there are a number of functions that could be implemented in mal itself (and some special forms that could be implemented in mal itself using macros). But the primary goal of mal is pedagogical (learning tool). That goal has two parts: 1. learning about Lisp by implementing a Lisp interpreter, 2. learning about the host language by implementing a significant application in that language. The choices about the order of the steps and which functions are implemented in mal vs in the host language is a balance between those two learning goals. In each step I try and introduce a core Lisp concept and also leverage some new aspects of the host language. If the goal was only learning Lisp then the order of steps 5-A would be very different: macros and try/cache would be very early so that those could be used to implement many of the core functions and some of the special forms directly in mal itself. But the current order and when/how functions are implemented is trying to balance those two learning goals. For example, |
@kanaka I did the following tests:
|
Part of
|
@OldLiu001 looks like runtest is now working for vbs using
but runtest is expecting them to be in correct order (the order the implementation wrote them):
This likely means that stderr is buffered and stdout is not. That's pretty typical even in Linux/macos. That's one of the things that the pty mode addresses (in addition to allowing line editing i.e. libedit/GNU readline, to be tested properly). There are a few ways to address that:
|
@kanaka Thank you for your precise point of view. I wrote
The results show that there is indeed a problem with standard error streams being buffered, and your sense is keen! I looked through the vbs manual and found no information about refreshing the output cache. Then I test it on
I was surprised to find that the results were returned in the correct order. Next I tried to fix it with:
But it didn't work. Combined with the above information, please feel free to advise. I will also continue to search for information from the Internet, and if the cache problem is still not solved, then I will consider using |
I avoided the buffered stderr problem by introducing
Later, I found that introducing Now we have a new problem where
Again, the Bash impl output for debug:
|
@kanaka The Finally get rid of the extra |
I tried:
Although it will not continue to stuck in the More logs:
Not sure how to fix it, and would like your comments and suggestions. |
I think both failures are related to when newline are being printed out and the fact that when in test mode the user's input is not being echo'd (which would include a final newline). For example, I think the first failure is caused by this check in your REPL routine:
I think that is preventing a newline in cases where the eval result is empty. For the second failure related to the readline core function, I suspect there is again a newline issue but I'm not exactly sure from the result what's going on. Can you try adding a newline prior to every prompt? Something like this:
You won't necessarily want that for interactive repl usage, but it might be enlightening to figure out what's going on with the tests. |
@kanaka Accurate insight! |
@kanaka I forcibly overwrote my master branch using the |
@OldLiu001 Merged! Thanks for your work on this and pushing through even though it was tedious. Hopefully any future implementations that support Windows will have a smoother path because they can refer to what you did to get things working. Any plans for future implementations? Want to try getting the powershell implementation running/testing in Windows (i.e. adding a new powershell line to |
@kanaka Thank you for your patience. |
Hello everyone, maintainers & contributors!
After more than half a year, I finally implement it.
During this period, I benefited a lot and felt the charm of Lisp language. Thank you for the project and its contributors.
Unfortunately, it seems that the makefile file and python script cannot work on Windows, and Vbscript cannot run in linux.
But I have manually tested all the examples in the tests folder and make sure I passed them (run mal impl in VBScript are also passed).
Nevertheless, I still want to contribute my learning process and achievements to this project to attract future generations.
The readme file has already updated for others to run my script.